[circt-opt] Add pass --convert-core-to-fsm
#8852
Conversation
TaoBi22
changed the title
--convert-core-to-fsm--convert-core-to-fsm
| moduleOp.walk([&](circt::seq::CompRegOp reg) { | ||
| if (reg.getName()->contains("state")) { | ||
| stateRegs.push_back(reg); | ||
| } else { | ||
| variableRegs.push_back(reg); | ||
| } | ||
| }); | ||
| if (stateRegs.empty()) { | ||
| emitError(moduleOp.getLoc()) | ||
| << "Cannot find state register in this FSM. You might need to " | ||
| "manually specify which registers are state registers.\n"; | ||
| return mlir::failure(); | ||
| } |
There was a problem hiding this comment.
This analysis does not look reliable. Is it possible to replace it with an analogue of fsm_detect from yosys?
There was a problem hiding this comment.
Yes, that's true, but I think that to have modularity, it would be fine to split up the total pipeline into fsm_detect and fsm_extract (like Yosys does). So, theoretically there could be a pass circt-opt --annotate-fsm-state-registers that works in combination with circt-opt --convert-core-to-fsm. In the meantime, it's not too difficult for the user to manually annotate the FSM state registers in an RTL design.
There was a problem hiding this comment.
Maybe for now we could just require it to always have the name of the state register explicitly given by the user? That way we don't need to worry about users having it opaquely pick up a register they don't expect, and we can always later change it to a flag that will get it to look for an attribute
There was a problem hiding this comment.
Sorry for the big delay in getting time to properly review this @AtticusKuhn! If you don't have time to work towards getting this in then let me know and we can work something out.
I've left some comments below - the additional thing is that for a new pass we need to add some FileCheck/verify-diagnostics tests, both to make sure it works and to make sure we see errors when we expect to. You should find examples of this on other PRs that added new passes, but let me know if you have any questions about it!
| #include "llvm/Support/LogicalResult.h" | ||
| #include "llvm/Support/raw_ostream.h" | ||
|
|
||
| #include "circt/Conversion/CoreToFSM.h" |
There was a problem hiding this comment.
Repeated include (I'm surprised clang-format didn't catch this - I suspect it's due to having a space in the middle of the include list, which it would also be nice to drop)
| size_t currentValue); | ||
|
|
||
| static void getPossibleValues(llvm::DenseSet<size_t> &possibleValues, Value v) { | ||
| if (circt::hw::ConstantOp c = |
There was a problem hiding this comment.
nit:
| if (circt::hw::ConstantOp c = | |
| if (ConstantOp c = |
There was a problem hiding this comment.
actually can probably just be an auto?
| possibleValues.insert(c.getValueAttr().getValue().getZExtValue()); | ||
| return; | ||
| } | ||
| if (circt::comb::MuxOp m = dyn_cast_or_null<comb::MuxOp>(v.getDefiningOp())) { |
| llvm::DenseSet<size_t> &finalPossibleValues, size_t operandIdx, | ||
| size_t currentValue); | ||
|
|
||
| static void getPossibleValues(llvm::DenseSet<size_t> &possibleValues, Value v) { |
There was a problem hiding this comment.
Nit: the name of this function makes it sound like a getter, but it's a void. Maybe addPossibleValues or something would make more sense?
| if (numStates > 256) { // Heuristic threshold | ||
| // If the search space is too large, we abandon the analysis for this | ||
| // path. The outer function will fall back to its own full-range | ||
| // default. | ||
| return; |
There was a problem hiding this comment.
Would it make sense to emit a warning/error here?
| mlir::Block &newGuardBlock = guardRegion.front(); | ||
| Operation *terminator = newGuardBlock.getTerminator(); | ||
| hw::OutputOp hwOutputOp = dyn_cast<hw::OutputOp>(terminator); | ||
| assert(hwOutputOp && "Expected terminator to be hw.OutputOp"); |
There was a problem hiding this comment.
| assert(hwOutputOp && "Expected terminator to be hw.OutputOp"); | |
| assert(hwOutputOp && "Expected terminator to be hw.output op"); |
| reg.erase(); | ||
| } | ||
| Operation *terminator = actionRegion.back().getTerminator(); | ||
| hw::OutputOp hwOutputOp = dyn_cast<hw::OutputOp>(terminator); |
There was a problem hiding this comment.
This cast looks unnecessary
| clonedRegOp->erase(); | ||
| } | ||
|
|
||
| // delete the arguments from the output block |
There was a problem hiding this comment.
This comment looks like it's describing something other than what follows it
| if (failed(converged)) { | ||
| stateOp.emitError("Failed to canonicalize the generated state op"); | ||
| return failure(); | ||
| } |
There was a problem hiding this comment.
It looks like we can probably catch this after the first call directly?
| hw::ConstantOp guardConst = | ||
| transition.getGuardReturn() | ||
| .getOperand() | ||
| .getDefiningOp<circt::hw::ConstantOp>(); |
There was a problem hiding this comment.
is this guaranteed not to return null?
|
Thanks for the feedback @TaoBi22 . I'm somewhat busy currently, but I'll be sure to work through and address your feedback in the coming days. |
5 participants
This PR adds a pass to
circt-optcalled--convert-core-to-fsm. This pass converts code written in RTL (comb+seq) and converts it to thefsmdialect.It recognises and extracts FSM structure from an RTL netlist. It has similar functionality to
fsm_extractin yosys:https://yosyshq.readthedocs.io/projects/yosys/en/0.46/cmd/fsm.html